Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding mods to derive PRECT #260

Merged
merged 9 commits into from
Nov 3, 2023
Merged

Conversation

bstephens82
Copy link
Contributor

from PRECC+PRECL if PRECT is not in the cam.h0 files. This code could be extended to other variables.

@brianpm
Copy link
Collaborator

brianpm commented Sep 26, 2023

Just to note that #240 is a similar modification to allow RESTOM to be derived. The implementation there is quite different. I'd recommend we encourage doing these kinds of simple derivations in a consistent manner. I like @bstephens82 's approach of just having a derive_variable method. I'm not sure how I feel about doing the derivation with ncap2, but I'm open to it if others think this is better than using python tools.

@bstephens82
Copy link
Contributor Author

Just to note that #240 is a similar modification to allow RESTOM to be derived. The implementation there is quite different. I'd recommend we encourage doing these kinds of simple derivations in a consistent manner. I like @bstephens82 's approach of just having a derive_variable method. I'm not sure how I feel about doing the derivation with ncap2, but I'm open to it if others think this is better than using python tools.

Ah, I had heard RESTOM might be another variable this could be applied to, but didn't realize it had already been implemented. I figured for each variable that could be derived, we would need to add another "recipe" in derive_variable. In the case of PRECT using ncap2 was just one of the shortest routes I could come up with. Feel free to modify or suggest other methods. Is the idea to avoid using NCO operators where possible?

@brianpm
Copy link
Collaborator

brianpm commented Sep 26, 2023

Yeah, I think adding "recipes" for each variable is the right way to go.

We do use NCO to make time series files, but we have discussed that having NCO as a dependency might not be great for portability. I'm not opposed to using it in the short term, but we might need to revisit in the future.

I guess my point was mostly to note that we should somehow combine this PR and #240 to allow both PRECT and RESTOM to be generated when the requirements are met.

@brianpm
Copy link
Collaborator

brianpm commented Oct 3, 2023

I also notice that #232 mentions similar functionality, so should also be harmonized with this one and #240.

@justin-richling
Copy link
Collaborator

Also I believe this is mentioned (PRECT specifically) in Julie's issue #228, so hopefully this can close that issue out as well

@nusbaume nusbaume self-requested a review October 19, 2023 19:15
@nusbaume nusbaume added the enhancement New feature or request label Oct 19, 2023
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @bstephens82! It looks good, but I do have a few changes requests, mostly to avoid possible errors showing up if certain variables aren't present.

lib/adf_diag.py Outdated Show resolved Hide resolved
lib/adf_diag.py Outdated Show resolved Hide resolved
Comment on lines +941 to +942
Derive variables acccording to steps given here. Since derivations will depend on the
variable, each variable to derive will need its own set of steps below.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is totally fine for now, but long-term I wonder if we can figure out a way to add the derivation equations to the variable defaults YAML file itself, and then simply have this function properly parse those equations to produce the derived variable. That way future users won't have to modify the core ADF python code if they want to add a new derived variable.

Thoughts? @brianpm @justin-richling

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea...I am not sure how to implement it though.

lib/adf_diag.py Show resolved Hide resolved
lib/adf_diag.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there! Just a little bit of extra clean-up on your new file-checking code.

lib/adf_diag.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super-close, just one last question.

lib/adf_diag.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@nusbaume nusbaume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, thanks! Once @justin-richling also reviews and approves it then it will be ready to be merged.

Copy link
Collaborator

@justin-richling justin-richling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good on my end, thanks @bstephens82! This will help get us in the right direction for more derived quantities in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants